[libc][pthread] fix -Wmissing-field-initializers #126314

nickdesaulniers · 2025-02-07T21:42:00Z

Fixes:

llvm-project/libc/test/integration/src/pthread/pthread_rwlock_test.cpp:59:29:
warning: missing field '__preference' initializer
[-Wmissing-field-initializers]
   59 |   pthread_rwlock_t rwlock = PTHREAD_RWLOCK_INITIALIZER;
      |                             ^

Also, add a test that demonstrates the same issue for
PTHREAD_MUTEX_INITIALIZER, and fix that, too.

PTHREAD_ONCE_INIT does not have this issue and does have test coverage.

Fixes: llvm-project/libc/test/integration/src/pthread/pthread_rwlock_test.cpp:59:29: warning: missing field '__preference' initializer [-Wmissing-field-initializers] 59 | pthread_rwlock_t rwlock = PTHREAD_RWLOCK_INITIALIZER; | ^ Also, add a test that demonstrates the same issue for PTHREAD_MUTEX_INITIALIZER, and fix that, too. PTHREAD_ONCE_INIT does not have this issue and does have test coverage.

llvmbot · 2025-02-07T21:42:33Z

@llvm/pr-subscribers-libc

Author: Nick Desaulniers (nickdesaulniers)

Changes

Fixes:

llvm-project/libc/test/integration/src/pthread/pthread_rwlock_test.cpp:59:29:
warning: missing field '__preference' initializer
[-Wmissing-field-initializers]
   59 |   pthread_rwlock_t rwlock = PTHREAD_RWLOCK_INITIALIZER;
      |                             ^

Also, add a test that demonstrates the same issue for
PTHREAD_MUTEX_INITIALIZER, and fix that, too.

PTHREAD_ONCE_INIT does not have this issue and does have test coverage.

Full diff: https://github.com/llvm/llvm-project/pull/126314.diff

2 Files Affected:

(modified) libc/include/llvm-libc-macros/pthread-macros.h (+2-2)
(modified) libc/test/integration/src/pthread/pthread_mutex_test.cpp (+4)

diff --git a/libc/include/llvm-libc-macros/pthread-macros.h b/libc/include/llvm-libc-macros/pthread-macros.h
index 8a144dbd2e611e7..2b1440f9308ca7f 100644
--- a/libc/include/llvm-libc-macros/pthread-macros.h
+++ b/libc/include/llvm-libc-macros/pthread-macros.h
@@ -25,8 +25,8 @@
 #define PTHREAD_PROCESS_PRIVATE 0
 #define PTHREAD_PROCESS_SHARED 1
 
-#define PTHREAD_MUTEX_INITIALIZER {0}
-#define PTHREAD_RWLOCK_INITIALIZER {0}
+#define PTHREAD_MUTEX_INITIALIZER {}
+#define PTHREAD_RWLOCK_INITIALIZER {}
 
 // glibc extensions
 #define PTHREAD_STACK_MIN (1 << 14) // 16KB
diff --git a/libc/test/integration/src/pthread/pthread_mutex_test.cpp b/libc/test/integration/src/pthread/pthread_mutex_test.cpp
index ce2a3538924da84..ec8488da4ead98f 100644
--- a/libc/test/integration/src/pthread/pthread_mutex_test.cpp
+++ b/libc/test/integration/src/pthread/pthread_mutex_test.cpp
@@ -186,6 +186,10 @@ void multiple_waiters() {
   LIBC_NAMESPACE::pthread_mutex_destroy(&counter_lock);
 }
 
+// Test the initializer
+[[gnu::unused]]
+static pthread_mutex_t test_initializer = PTHREAD_MUTEX_INITIALIZER;
+
 TEST_MAIN() {
   relay_counter();
   wait_and_step();

github-actions · 2025-02-07T21:45:32Z

✅ With the latest revision this PR passed the C/C++ code formatter.

nickdesaulniers · 2025-02-07T21:46:52Z

oh gawd clang-format...my eyes!!!

jhuber6 · 2025-02-07T21:48:04Z

Do we really need a macro that uses 10x as many characters as it's replacing.

nickdesaulniers · 2025-02-07T21:48:54Z

Homey, that boat sailed 100 years ago. That feedback should have been to the austin group back when they made up this shit!

nickdesaulniers · 2025-02-07T21:51:41Z

Also, I assume that as opaque types, you could perhaps have non-zero values you'd like to initialize these members to, so declaring static pthread_rwlock_t lock; would produce a pthread_rwlock_t with DIFFERENT values than one initialized via pthread_rwlock_init (static pthread_rwlock_t lock = PTHREAD_RWLOCK_INITIALIZER; being the fix, IIUC).

They probably also made this up BEFORE they had designated initializers; gotta love aggregate initializers! Don't mess up the order ever! Refactor struct definitions at your peril!

nickdesaulniers · 2025-02-07T22:57:33Z

oh gawd clang-format...my eyes!!!

So, echo '#define foo {}' | clang-format - can produce different output based on where in the tree you are. IIUC, clang-format will traverse up until it finds a .clang-format. We have effectively echo '#define foo {}' | clang-format --style='{BasedOnStyle: LLVM}' - due to llvm-project/.clang-format.

But there may be a format option that fixes this (who knows if it's specific enough though, or too broad). Still digging in https://clang.llvm.org/docs/ClangFormatStyleOptions.html.

nickdesaulniers · 2025-02-07T22:58:56Z

echo '#define foo {}' | clang-format --style='{SkipMacroDefinitionBody: true}' - does it; probably don't want that generally though.

frobtech · 2025-02-08T01:07:37Z

Homey, that boat sailed 100 years ago. That feedback should have been to the austin group back when they made up this shit!

This is moot anyway, because you can't use {} in C (maybe C23 allows it or something, but not C99 without GNU extensions, definitely). Short of using pragma magic to just suppress the warning (which I'm not even sure you can do in this context), you just have to actually have a value listed for each member and the right number of commas.

lntue · 2025-02-08T06:02:24Z

libc/test/integration/src/pthread/pthread_mutex_test.cpp

@@ -186,6 +186,10 @@ void multiple_waiters() {
  LIBC_NAMESPACE::pthread_mutex_destroy(&counter_lock);
 }

+// Test the initializer
+[[gnu::unused]]


Why not [[maybe_unused]]?

good catch! I always forget that that one got standardized.
bfc84ca

nickdesaulniers · 2025-02-10T17:31:50Z

This is moot anyway, because you can't use {} in C (maybe C23 allows it or something, but not C99 without GNU extensions, definitely).

You're right. https://godbolt.org/z/fnYExjrf5 Aggregate initialization (peasantry) it is, then. God, why have you forsaken me?

nickdesaulniers · 2025-02-10T17:55:21Z

Let me file an issue about our pthread_rwlock_t being linux specific, which has implications for the initializer.

michaelrj-google

the code LGTM, the formatting seems like a pain

michaelrj-google · 2025-02-11T22:14:26Z

libc/include/llvm-libc-macros/pthread-macros.h

+      /* .__is_pshared = */ 0,                                                 \
+      /* .__preference = */ 0,                                                 \
+      /* .__state = */ 0,                                                      \
+      /* .__write_tid = */ 0, /* .__wait_queue_mutex = */                      \


do you want these two comments to be on separate lines?

Yes; but dunno how to fix this quickly with clang-format.

does it work if you make it like:

/* .__wait_queue_mutex = */ {0}, \ /* .__pending_readers = */ {0}, \ ...

It does. That makes me sad to mix comments that look like designated initializers, but only for top level members, with more aggregate initializers without such comments for nested members, but I lack the will to wrestle with the formatter.

I think we could put a .clang-format file in this dir, and use https://clang.llvm.org/docs/ClangFormatStyleOptions.html#whitespacesensitivemacros to turn off formatting for this macro.

2da1ce2 PTAL

libc/include/llvm-libc-macros/pthread-macros.h

Fixes: llvm-project/libc/test/integration/src/pthread/pthread_rwlock_test.cpp:59:29: warning: missing field '__preference' initializer [-Wmissing-field-initializers] 59 | pthread_rwlock_t rwlock = PTHREAD_RWLOCK_INITIALIZER; | ^ Also, add a test that demonstrates the same issue for PTHREAD_MUTEX_INITIALIZER, and fix that, too. PTHREAD_ONCE_INIT does not have this issue and does have test coverage.

nickdesaulniers requested review from frobtech and michaelrj-google February 7, 2025 21:42

llvmbot added the libc label Feb 7, 2025

michaelrj-google approved these changes Feb 7, 2025

View reviewed changes

lntue reviewed Feb 8, 2025

View reviewed changes

nickdesaulniers marked this pull request as draft February 10, 2025 17:32

nickdesaulniers added 2 commits February 10, 2025 09:52

switch to aggregate initialization

c380f97

s/gnu::unused/maybe_unused/

bfc84ca

nickdesaulniers marked this pull request as ready for review February 10, 2025 17:55

nickdesaulniers mentioned this pull request Feb 10, 2025

[libc][pthread] pthread_rwlock_t is linux specific #126559

Open

nickdesaulniers added 2 commits February 10, 2025 09:58

reformat

868122b

fix include path

0f16df9

nickdesaulniers requested review from lntue and michaelrj-google February 10, 2025 19:01

michaelrj-google approved these changes Feb 11, 2025

View reviewed changes

nickdesaulniers commented Feb 11, 2025

View reviewed changes

libc/include/llvm-libc-macros/pthread-macros.h Show resolved Hide resolved

nickdesaulniers added 2 commits February 11, 2025 14:17

cmake

ff3e6d9

attempt reformat

2da1ce2

michaelrj-google approved these changes Feb 12, 2025

View reviewed changes

nickdesaulniers merged commit 3e02069 into llvm:main Feb 12, 2025
13 checks passed

nickdesaulniers deleted the missing-field-initializers branch February 12, 2025 22:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc][pthread] fix -Wmissing-field-initializers #126314

[libc][pthread] fix -Wmissing-field-initializers #126314

nickdesaulniers commented Feb 7, 2025

llvmbot commented Feb 7, 2025

github-actions bot commented Feb 7, 2025 •

edited

Loading

nickdesaulniers commented Feb 7, 2025

jhuber6 commented Feb 7, 2025

nickdesaulniers commented Feb 7, 2025

nickdesaulniers commented Feb 7, 2025 •

edited

Loading

nickdesaulniers commented Feb 7, 2025

nickdesaulniers commented Feb 7, 2025

frobtech commented Feb 8, 2025

lntue Feb 8, 2025

nickdesaulniers Feb 10, 2025

nickdesaulniers commented Feb 10, 2025

nickdesaulniers commented Feb 10, 2025

michaelrj-google left a comment

michaelrj-google Feb 11, 2025

nickdesaulniers Feb 11, 2025

lntue Feb 11, 2025

nickdesaulniers Feb 12, 2025

nickdesaulniers Feb 12, 2025

[libc][pthread] fix -Wmissing-field-initializers #126314

[libc][pthread] fix -Wmissing-field-initializers #126314

Conversation

nickdesaulniers commented Feb 7, 2025

llvmbot commented Feb 7, 2025

github-actions bot commented Feb 7, 2025 • edited Loading

nickdesaulniers commented Feb 7, 2025

jhuber6 commented Feb 7, 2025

nickdesaulniers commented Feb 7, 2025

nickdesaulniers commented Feb 7, 2025 • edited Loading

nickdesaulniers commented Feb 7, 2025

nickdesaulniers commented Feb 7, 2025

frobtech commented Feb 8, 2025

lntue Feb 8, 2025

Choose a reason for hiding this comment

nickdesaulniers Feb 10, 2025

Choose a reason for hiding this comment

nickdesaulniers commented Feb 10, 2025

nickdesaulniers commented Feb 10, 2025

michaelrj-google left a comment

Choose a reason for hiding this comment

michaelrj-google Feb 11, 2025

Choose a reason for hiding this comment

nickdesaulniers Feb 11, 2025

Choose a reason for hiding this comment

lntue Feb 11, 2025

Choose a reason for hiding this comment

nickdesaulniers Feb 12, 2025

Choose a reason for hiding this comment

nickdesaulniers Feb 12, 2025

Choose a reason for hiding this comment

github-actions bot commented Feb 7, 2025 •

edited

Loading

nickdesaulniers commented Feb 7, 2025 •

edited

Loading